Begin implementation for native histograms #3789
Conversation
zalegrala
force-pushed
the
native-histograms-getting-started
branch
from
2f53a6d
to
490b7b4
Compare
zalegrala
force-pushed
the
native-histograms-getting-started
branch
from
9d33bdf
to
1bed650
Compare
zalegrala
requested review from
joe-elliott,
annanay25,
mdisibio,
mapno,
yvrhdn,
electron0zero,
ie-pham and
stoewer
as code owners
|
|
||
| func (h *nativeHistogram) activeSeriesPerHistogramSerie() uint32 { | ||
| // TODO can we estimate this? | ||
| return 1 |
There was a problem hiding this comment.
this is interesting. i suppose this plays into how many series prom/mimir consider a native histogram?
There was a problem hiding this comment.
Right, I think we have something to learn here, since there doesn't seem to be a clean mapping between native histograms and active series.
| if err != nil { | ||
| return activeSeries, err | ||
| } | ||
| s.histogram = encodedMetric.GetHistogram() |
There was a problem hiding this comment.
is there a reason to store this as a field on the struct? it seems like we only use it in this method. if that's true we should remove it from the struct
There was a problem hiding this comment.
This was added so that we could remember which histograms were sent. The Reset call below captures this, and storing the state on the struct was a way for us to get around this issue. If we don't know which buckets have had their exemplars reported, they get updated with the most recent timestamp and sent again, since the series keeps the exemplars.
There was a problem hiding this comment.
yeah, i'm not seeing this. it seems only referenced in this method which makes me think we should just use a local var. perhaps i'm missing something. we can sync on this.
| // NOTE: A subtle point here is that the new histogram implementation is | ||
| // chosen in the registry. This means that if the user overrides change | ||
| // from "both" to "classic", we will still be using the new histograms | ||
| // implementation. This is because the registry is not recreated when the | ||
| // overrides change. This is a tradeoff to avoid recreating the registry | ||
| // for every change in the overrides, but it does mean that even if user | ||
| // changes to "classic" histograms, we will still be using the new | ||
| // histograms implementation and want to avoid generating native | ||
| // histograms. |
There was a problem hiding this comment.
This will work when we switch from native or both to classic, but it will not work when we go from classic to anything else. If we initiate their tenant with classic we use the old histogram implementation, when we change the override nothing will be updated and we keep using this implementation.
To workaround this we need to recreate the processors I think so they all re-register their histograms.
But maybe something quick to fix in a follow-up PR?
There was a problem hiding this comment.
Right, I think let's follow up.
What this PR does:
Here we add what is necessary to generate and remote-write native histograms from the metrics-generator.
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]